-
Notifications
You must be signed in to change notification settings - Fork 269
Add machine type in zone validator #5099
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Add machine type in zone validator #5099
Conversation
Summary of ChangesHello @saara-tyagi27, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the HPC Toolkit's blueprint validation process by adding a new validator to ensure that all specified Google Cloud machine types are valid and available within their designated zones. This proactive check helps users catch potential configuration errors early, preventing deployment failures and improving the overall reliability of blueprint definitions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new validator to verify that a specified machine type is available in a given zone. This is a valuable addition for preventing deployment failures due to misconfigurations. The implementation correctly iterates through modules, identifies machine type settings, and validates them against the Compute Engine API. My review includes a couple of suggestions to improve a warning message and enhance code formatting for better readability and maintainability.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new and valuable validator, test_machine_type_in_zone, to verify Google Cloud machine type availability before deployment. The implementation is well-executed, with automatic discovery of machine types and graceful handling of environmental issues like API or permission errors through a new soft warning mechanism. The accompanying documentation and test updates are thorough and clear. My review includes one suggestion to improve the design of the new warning handling logic for better testability and separation of concerns, in line with the project's goals for technical excellence and maintainability.
| inputs: | ||
| project_id: $(vars.project_id) | ||
| zone: $(vars.zone) | ||
| machine_type: c2-standard-60 # any machine type to verify in the zone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hardcoded machines type here..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As machine_type is not a global variable in most blueprints, I specified a hardcoded one. But if a global variable is defined that can be referenced as well.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This is a well-executed pull request that introduces a valuable new validator, test_machine_type_in_zone. The implementation is robust, featuring automatic discovery of machine types, intelligent handling of zone overrides, and a thoughtful approach to API errors using soft warnings to avoid blocking deployments unnecessarily. The addition of comprehensive unit tests using a mocked compute service is excellent and ensures the new logic is reliable. The documentation is also clear and thorough. I have one minor suggestion to improve code consistency.
Summary
This PR introduces a new validator, test_machine_type_in_zone validator ensures Google Cloud machine type availability via the Compute Engine API prior to deployment. The logic automatically identifies and validates settings matching machine_type suffix against zone across all blueprint modules.
Key Features
Warning Handling
A new helper function, handleSoftWarning, has been implemented to manage environment-related API errors.
Documentation & Testing